Skip to content

Add support for NOT NULL columns and foreign keys with ON DELETE#36

Merged
dpurgin merged 5 commits intodpurgin:masterfrom
maciek134:feat/ut-nonnull-fk-on-delete
Mar 16, 2026
Merged

Add support for NOT NULL columns and foreign keys with ON DELETE#36
dpurgin merged 5 commits intodpurgin:masterfrom
maciek134:feat/ut-nonnull-fk-on-delete

Conversation

@maciek134
Copy link
Copy Markdown
Contributor

@maciek134 maciek134 commented Jan 29, 2026

Hello, hope you don't mind contributions without prior discussion - I've added two features I needed. I tried to follow the current code style and approach.

Oh and I've put them both in one PR because they interact, but if you prefer squashing PRs I'd be happy to put them separately (otherwise I have no problems rebasing my changes after any comments you might have to enable a fast-forward merge).

The FK on relations is implicit - gets created if ON_DELETE is set. This felt in-line with the current approach, but I'm happy to make it explicit too.

@maciek134 maciek134 force-pushed the feat/ut-nonnull-fk-on-delete branch from 674c984 to d2ae3f8 Compare January 29, 2026 12:38
@dpurgin
Copy link
Copy Markdown
Owner

dpurgin commented Feb 4, 2026

Czesc Macieju,

sorry for the late reply and thanks for the contribution!

Could you please update README.MD, the section "Mapping customization" with the new keywords?

You can freely use C++17 and [[nodiscard]] instead of Q_REQUIRED_RESULT. The reason for Q_REQUIRED_RESULT was that I had not decided for C++17 yet in one of the earlier QtOrm prototypes.

The NOT_NULL addition is a very useful one but I have a conceptual concern about the cascading.

The reason why I haven't had the foreign key support, especially with cascading, is that the entity cache in the OR-mapper has to be updated as well on the cascade action. Imagine you have the following:

| Provinces                | 
|--------------------------|
| id    |  name            |
|-------|------------------|
| 4     |  Upper Austria   |
|-------|------------------|

| Towns                                 |  
|---------------------------------------|
| id      |  province_id       |  name  |
|---------|--------------------|--------|
| 1       |  4                 |  Linz  |

Now, I read all provinces and towns in the OR-Mapper, and both class instances are created as C++ objects and cached inside the OR mapper in QOrmEntityInstanceCache. And then I execute from<Province>().filter(Q_ORM_CLASS_PROPERTY(name) == "Linz").remove(). The problem here is that we do not know which rows are actually removed from the database, and which referenced entities are affected.

To deal with the first part, I had a prototype implementation of QOrmSqliteProvider::remove performing a helper SELECT id query with the same filter as the remove to find out the cache instances to clean up, but then I dropped it for some reason.

Then I mitigated the problem at least for SQLite > 3.35, because there is a RETURNING clause which is used in QOrmSqliteProvider::remove() to cleanup the cache of Provinces.

The second part of the problem is that we have to find out which entities are referenced by the removed ones and purge them from cache as well. Without the cascading it is the programmer's job to cleanup the referenced Towns before even executing the remove query. Now with cascading it becomes even trickier, and probably requires the secondary SELECT again.

If the OR mapper already does the cascading to clean up its cache, then ON DELETE in the FK definition becomes unneccessary:

  • if the OR mapper works correctly, it would do nothing, because the referenced rows would be removed by the OR mapper
  • if the OR mapper does not work correctly and skips some of the references, then ON DELETE CASCADE would "silently" remove the referenced rows from the database, and the OR mapper state would become inconsistent. In this case I would rather prefer a foreign key violation error from the database.

What are your thoughts on this?

Cheers & dzieki bardzo

@maciek134
Copy link
Copy Markdown
Contributor Author

Cześć :) Sorry for the late response too, I was sure I checked here and somehow missed it.

That makes perfect sense, I completely missed the issue (good thing you caught it instead of me finding out after using it 😅 ). Cascade at DB level doesn't make much sense, I'll remove it.

ORM-level cascade would be rather hard to fit with the current API, because remove returns the deleted entities. Technically easy to solve with indirection, but then the programmer would have to expect it.

I think foreign keys for data integrity would still be good though, right?

@maciek134 maciek134 force-pushed the feat/ut-nonnull-fk-on-delete branch 8 times, most recently from ec455d6 to a226fe1 Compare March 12, 2026 21:48
@maciek134 maciek134 marked this pull request as draft March 12, 2026 22:33
@maciek134 maciek134 force-pushed the feat/ut-nonnull-fk-on-delete branch from a226fe1 to 6ddc724 Compare March 13, 2026 20:33
@maciek134 maciek134 marked this pull request as ready for review March 13, 2026 20:34
@maciek134 maciek134 force-pushed the feat/ut-nonnull-fk-on-delete branch from 6ddc724 to 9cbc66f Compare March 13, 2026 20:52
@maciek134
Copy link
Copy Markdown
Contributor Author

maciek134 commented Mar 13, 2026

Ok, @dpurgin I've updated the changes. In the meantime I needed UNIQUE constraints too and found two escape issues - happy to split off into another one if you want to keep this PR smaller.

NOT_NULL could be supported for ALTER TABLE (Append schema mode) but that would require also supporting DEFAULT so for now I just mentioned in the README that this won't work.

EDIT: sorry for drafting again, looks like I messed up a rebase

@maciek134 maciek134 marked this pull request as draft March 13, 2026 21:18
@maciek134 maciek134 force-pushed the feat/ut-nonnull-fk-on-delete branch from 9cbc66f to 095bca7 Compare March 13, 2026 21:44
@maciek134 maciek134 marked this pull request as ready for review March 13, 2026 21:45
@dpurgin
Copy link
Copy Markdown
Owner

dpurgin commented Mar 16, 2026

Thanks for your work! I've tested it with other projects and it looks good!

@dpurgin dpurgin merged commit 7a8a179 into dpurgin:master Mar 16, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants